Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #4743: Arr::path() to return associative array when wildcard used #352

Open
wants to merge 2 commits into
base: 3.3/develop
Choose a base branch
from

Conversation

tommcdo
Copy link

@tommcdo tommcdo commented May 15, 2013

@enov
Copy link
Contributor

enov commented Nov 21, 2014

This is a well written bug fix, with tests. However, existing code relying on the numeric sequential keys of the output of this function will break, if we apply this fix.

@acoulton could you kindly comment what to do in this case?

@acoulton
Copy link
Member

Tricky. Since it changes the response, I think it would have to go to 3.4 with a clear note in the upgrade guide.

It will only be if end-user code is specifically looking at the keys, I think, that it's an issue - they'll still come out in the same sequence as far as I can see? It's probably quite an edge case, but still not safe for 3.3.

@tommcdo if you disagree we'd welcome your opinion.

@tommcdo
Copy link
Author

tommcdo commented Nov 21, 2014

I think it's quite unlikely that users are currently relying on the numerical keys with the current implementation, since they're quite frankly not useful at all. However, this is not strictly a bug fix, so it's perfectly reasonable to hold off until 3.4.

@enov
Copy link
Contributor

enov commented Nov 22, 2014

However, this is not strictly a bug fix, so it's perfectly reasonable to hold off until 3.4.

@tommcdo could you please PR this to 3.4/develop branch.

@neo22s
Copy link
Member

neo22s commented Mar 21, 2016

What we do with this? to 4.0.0? Looks good to me.

@neo22s neo22s added this to the 4.0.0 milestone Mar 21, 2016
@acoulton
Copy link
Member

I suggest a contrib manually merges this PR to the 4.0 branch and adds to upgrade notes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants